Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(@angular/ssr): bundle Critters #28228

Merged
merged 4 commits into from
Aug 24, 2024

Conversation

alan-agius4
Copy link
Collaborator

@alan-agius4 alan-agius4 commented Aug 19, 2024

This commit bundles the Critters library to ensure compatibility with Nodeless environments. Additionally, all licenses for bundled libraries, including Critters, are now included in the package. This helps maintain compliance with open-source license requirements.

@alan-agius4 alan-agius4 added the target: major This PR is targeted for the next major release label Aug 19, 2024
@alan-agius4 alan-agius4 force-pushed the critters-bundled branch 10 times, most recently from f5950df to 2001375 Compare August 20, 2024 13:41
@alan-agius4 alan-agius4 added the action: review The PR is still awaiting reviews from at least one requested reviewer label Aug 20, 2024
@alan-agius4 alan-agius4 marked this pull request as ready for review August 20, 2024 14:03
@alan-agius4 alan-agius4 requested a review from dgp1130 August 21, 2024 17:08
@alan-agius4 alan-agius4 force-pushed the critters-bundled branch 5 times, most recently from 98b5dad to ec3823b Compare August 22, 2024 18:38
@dgp1130
Copy link
Collaborator

dgp1130 commented Aug 22, 2024

I can see that a decent chunk of this code was just moved around, so I tried not to comment too much on it. Feel free to ignore any pre-existing issues which should be out of scope for this PR.

@angular-robot angular-robot bot added the area: build & ci Related the build and CI infrastructure of the project label Aug 23, 2024
@alan-agius4 alan-agius4 force-pushed the critters-bundled branch 4 times, most recently from 17a0038 to 5d45ecd Compare August 23, 2024 09:09
@angular-robot angular-robot bot added area: build & ci Related the build and CI infrastructure of the project and removed area: build & ci Related the build and CI infrastructure of the project labels Aug 23, 2024
@alan-agius4 alan-agius4 requested a review from dgp1130 August 23, 2024 10:04
@alan-agius4 alan-agius4 force-pushed the critters-bundled branch 4 times, most recently from c0ec7dc to 14fa983 Compare August 23, 2024 11:52
This commit bundles the Critters library to ensure compatibility with Nodeless environments. Additionally, all licenses for bundled libraries, including Critters, are now included in the package. This helps maintain compliance with open-source license requirements.
This refactor adds a workaround to enable TypeScript declaration merging for the Critters class. We initially tried using interface merging on the default-exported Critters class:

```typescript
interface Critters {
  embedLinkedStylesheet(link: PartialHTMLElement, document: PartialDocument): Promise<unknown>;
}
```

However, since Critters is exported as a default class, TypeScript's declaration merging does not apply. To solve this, we introduced a new class, CrittersBase, which extends Critters, and added the required method in the CrittersBase interface:

```typescript
interface CrittersBase {
  embedLinkedStylesheet(link: PartialHTMLElement, document: PartialDocument): Promise<unknown>;
}
class CrittersBase extends Critters {}
```
- Implemented functionality to compare and update third-party license files
- Added handling for '--accept' argument to update the golden license file
- Included tests to ensure the Critters license file matches the golden reference

This update ensures license file consistency and provides an easy way to update the reference file when necessary.
Leverage the built-in `diff_test` feature from Bazel to check for file changes. For details, see: https://github.com/bazelbuild/bazel-skylib/blob/main/docs/diff_test_doc.md
@@ -29,7 +29,6 @@
"@angular/platform-server": "19.0.0-next.1",
"@angular/router": "19.0.0-next.1",
"@bazel/runfiles": "^5.8.1",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Would @bazel/runfiles make more sense in the root package.json? I'm kinda surprised it's not there already.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

out = "critters_license_file_accept.sh",
content =
[
"#!/usr/bin/env bash",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat trick with write_file, didn't know it could create a binary like that.

One limitation here is that this will only work for Unix developers, not Windows devs. It's probably not a huge deal, especially since anyone could manually copy over the text, even if that is a bit annoying.

Maybe once we get on @aspect_rules_js it might be worth making a more reusable and cross-platform golden_test macro. I thought @aspect_bazel_lib had something here, but it seems like their diff_test isn't much smarter than Skylib's.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could create a macro that creates a CMD instead a bash for Windows.

But as mentioned there are workaround, and likely the update of the golden will be done by one of team which at the moment use Linux or Mac.

@alan-agius4 alan-agius4 added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Aug 24, 2024
@alan-agius4 alan-agius4 merged commit ad65c3f into angular:main Aug 24, 2024
31 checks passed
@alan-agius4 alan-agius4 deleted the critters-bundled branch August 24, 2024 05:37
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: @angular/ssr area: build & ci Related the build and CI infrastructure of the project target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants